feat(evo-marko): evo-combobox#661
Conversation
🦋 Changeset detectedLatest commit: 3526052 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new evo-combobox tag to @evo-web/marko, including styling, Storybook docs/examples, and SSR/browser tests.
Changes:
- Introduces
evo-comboboxMarko implementation, styles, README, and Storybook stories/examples (including async filtering via JSON data). - Adds SSR snapshot coverage and browser interaction tests for core combobox behaviors.
- Updates TypeScript config to support JSON module imports and includes JSON in compilation scope.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/evo-marko/tsconfig.json | Enables JSON module support for Storybook/examples and includes JSON files in TS scope. |
| packages/evo-marko/src/tags/evo-combobox/index.marko | Implements the combobox UI, filtering, keyboard/mouse interactions, and markup/ARIA. |
| packages/evo-marko/src/tags/evo-combobox/style.ts | Adds Skin CSS imports required by the new combobox and related UI elements. |
| packages/evo-marko/src/tags/evo-combobox/combobox.stories.ts | Adds Storybook stories and controls for the new component. |
| packages/evo-marko/src/tags/evo-combobox/README.md | Documents usage, props, and attribute tags for evo-combobox. |
| packages/evo-marko/src/tags/evo-combobox/examples/*.marko | Adds examples (default, controllable, async filtering) used by Storybook. |
| packages/evo-marko/src/tags/evo-combobox/examples/data.json | Adds example dataset used by async filtering example. |
| packages/evo-marko/src/tags/evo-combobox/test/test.server.ts | Adds SSR snapshot tests for multiple variants. |
| packages/evo-marko/src/tags/evo-combobox/test/test.browser.ts | Adds browser interaction tests (focus, expand/collapse, keyboard, click, filtering). |
| packages/evo-marko/src/tags/evo-combobox/test/snapshots/test.server.ts.snap | Adds SSR snapshots. |
| CLAUDE.md | Adds Marko 6 guidance notes for extractor and event handler typing patterns. |
| .changeset/bright-socks-care.md | Publishes a patch changeset for @evo-web/marko. |
| } else if (e.key === "Enter") { | ||
| if (open && activeIndex >= 0) { | ||
| e.preventDefault(); | ||
| value = displayValue ?? visibleOptions[activeIndex].text; | ||
| displayValue = null; | ||
| } | ||
| open = false; | ||
| } else if (e.key === "Escape") { |
There was a problem hiding this comment.
Claude PR Review. Full list here: https://github.com/eBay/evo-web/tasks/fe6604f5-9c3b-4888-9e89-e3482af049e7
Three blocking issues that will cause test suite failures and a WCAG violation: both test files import stories that don't exist in the stories file (Filtering, FloatingLabel), and non-selected listbox options are missing the required aria-selected attribute. One non-blocking type annotation nit.
ArtBlue
left a comment
There was a problem hiding this comment.
Previous blocking issues are all resolved — great cleanup. One new blocking bug introduced by the tempValue refactor: Escape key doesn't clear tempValue, so keyboard-navigated values commit on blur. Three non-blocking observations: redundant aria-owns, always-in-DOM listbox relying on skin CSS, and lost SSR coverage.
| } | ||
| open = false; | ||
| } else if (e.key === "Escape") { | ||
| open = false; |
There was a problem hiding this comment.
Blocking: Escape closes the listbox but doesn't clear tempValue. Repro: arrow-navigate to an option (sets tempValue), press Escape (intending to cancel), then click away — onBlur sees tempValue is still set and commits it as the new value, overriding the user's intent to cancel.
Add tempValue = null here:
} else if (e.key === "Escape") {
open = false;
activeIndex = -1;
tempValue = null; // discard keyboard navigation on cancel
}
| value:=value | ||
| placeholder=showPlaceholder && placeholder | ||
| autocomplete="off" | ||
| aria-owns=listboxId |
There was a problem hiding this comment.
Non-blocking: aria-owns alongside aria-controls referencing the same element is redundant. The ARIA 1.2 combobox pattern specifies aria-controls to reference the popup listbox — aria-owns is only needed when the DOM parent-child relationship doesn't match the desired accessibility tree structure. Since the listbox is already a descendant of the same combobox widget, aria-controls alone is sufficient. Consider removing aria-owns.
| }/> | ||
| </if> | ||
| </span> | ||
| <div |
There was a problem hiding this comment.
Non-blocking: The listbox is now always in the DOM (the previous <if=open> guard was removed). AT visibility when the combobox is closed now depends entirely on the skin CSS using display: none for .combobox__listbox when .combobox--expanded is absent — display: none correctly removes the element from the accessibility tree; visibility: hidden or opacity: 0 would not. Worth confirming the skin CSS uses display for this toggle.
| }); | ||
|
|
||
| it("renders with fixed strategy", async () => { | ||
| await snapshotHTML(Default, { strategy: "fixed" }); |
There was a problem hiding this comment.
Non-blocking: The previous revision had SSR snapshots for floatingLabel and Filtering (autocomplete='list') modes — they were removed here rather than replaced. The snapshot count is now 6 cases vs. the prior 8. Consider adding:
it("renders floating label", async () => {
await snapshotHTML(Default, { floatingLabel: "Campaign" });
});
it("renders filtering variant", async () => {
await snapshotHTML(Default, { autocomplete: "list" });
});
Description
<evo-combobox>to@evo-web/markoScreenshots